-
Notifications
You must be signed in to change notification settings - Fork 161
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for Node.js version 22 #1779
Conversation
I don't use Node 22 isn't available on conda, so I haven't tested it, but have you tested it out locally and it's working for you? |
We can use the .nvmrc as single source of truth for CI versions of node as
well! Then we only need to change one string rather than one each step that
installs node.
It is not something we need to keep religiously up to date. It can be
bumped whenever we update versions, say once a year.
I tested with 22 and it worked locally with `npm run dev`
Also, the tests passed apparently.
…On Wed, Jun 5, 2024, 23:38 james hadfield ***@***.***> wrote:
I don't use nvm but the team recently discussed (slack)
<https://bedfordlab.slack.com/archives/C01LCTT7JNN/p1715363558192649> and
implemented <nextstrain/nextstrain.org#855>
such a file for nextstrain.org. Because I don't use it it'll invariably
fall out of sync, but those who do use it will hopefully keep it updated.
Are you using it, or was this added in an attempt to appease CI?
Node 22 isn't available on conda <https://anaconda.org/conda-forge/nodejs>,
so I haven't tested it, but have you tested it out locally and it's working
for you?
—
Reply to this email directly, view it on GitHub
<#1779 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AF77AQODVQ3B3H3HCVO346DZF6AO7AVCNFSM6AAAAABI22U6MGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJRGAYDIOJUGM>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
.nvmrc
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use the .nvmrc as single source of truth for CI versions of node as well
How would this work if CI needs to test a matrix but only one version is set in this file?
The single source of truth should be package.json, since that's what npm
checks when running scripts.
Line 10 in d6fd0aa
"node": "^16 || ^18 || ^20 || ^22", |
Ideally we use this to define the matrix in CI, but I don't think there's a way (at least not straightforward).
.nvmrc
Outdated
@@ -0,0 +1 @@ | |||
20 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The addition of .nvmrc
made sense for nextstrain.org since we only support a single version of Node.js there, and .nvmrc
serves to define that single version for devs who use nvm. However, Auspice supports a range of Node.js versions so there's no reason to constrain to a single version. I would remove this file.
.github/workflows/ci.yaml
Outdated
- run: gh workflow run ci.yml --repo nextstrain/docker-base | ||
env: | ||
GITHUB_TOKEN: ${{ secrets.GH_TOKEN_NEXTSTRAIN_BOT_WORKFLOW_DISPATCH }} | ||
- run: gh workflow run ci.yml --repo nextstrain/docker-base | ||
env: | ||
GITHUB_TOKEN: ${{ secrets.GH_TOKEN_NEXTSTRAIN_BOT_WORKFLOW_DISPATCH }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These unrelated formatting changes should not be part of this commit.
v22 is the current version, with scheduled end-of-life 2027-04-30¹. The associated npm version (as of node v22.2.0) is npm v10 (v.10.7.0 for v22.2.0)². ¹ https://github.com/nodejs/release#release-schedule ² https://nodejs.org/dist/index.json
I fixed up some unrelated changes introduced here and removed the Our CI is passing, including the build-docs step. Read the docs' automated build is failing, which is also seen on other PRs, but I don't think it's related to these changes. Merging. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅
v22 is the current version, with scheduled end-of-life 2027-04-30¹.
The associated npm version (as of node v22.2.0) is npm v10 (v.10.7.0 for v22.2.0)².
¹ https://github.com/nodejs/release#release-schedule
² https://nodejs.org/dist/index.json
Commit adapted from #1673
Checklist